-
Notifications
You must be signed in to change notification settings - Fork 501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ingest: Render captive core logs as Horizon logs #3189
Conversation
2deec7e
to
557fe71
Compare
We go from "abcde" [category LEVEL] stuff here getting rendered as LVEL[time] stuff here to LVEL[time] category: stuff here
1376e57
to
03fe249
Compare
All good points; I believe they're resolved now @tamirms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Minor comments and improvements ideas.
@@ -182,6 +182,8 @@ func NewSystem(config Config) (System, error) { | |||
cancel() | |||
return nil, errors.Wrap(err, "error creating captive core backend") | |||
} | |||
captiveCoreBackend.SetLogger(log) | |||
ledgerBackend = captiveCoreBackend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can leave the old code and write:
ledgerBackend.(*ledgerbackend.CaptiveStellarCore).SetLogger(log)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though less lines, is it cleaner? As someone newish to Go, it's quite a bit harder to read, especially with the new change about name + subservice, it becomes:
ledgerBackend.(*ledgerbackend.CaptiveStellarCore).SetStellarCoreLogger(log.WithField("subservice", "stellar-core"))
😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right!
} | ||
|
||
levelRx := regexp.MustCompile(`\[(\w+) ([A-Z]+)\]`) | ||
indices := levelRx.FindStringSubmatchIndex(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too complicated. If you use FindSubmatch
and rewrite the regexp like in https://play.golang.com/p/mYf-9iMkOCg you won't have to deal with cutting the string yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially did this, but the problem is that I still needed the index to trim the line at the end. To do the above, I'd unfortunately have to match twice; do we want to do this? The code is definitely cleaner but slightly less efficient; it's done in 7187374, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the problem is that I still needed the index to trim the line at the end
You don't have to if you change the regexp to G[A-Z]{4} \[(\w+) ([A-Z]+)\] (.*)
(as in play.golang example above). The line remaining is also a group match so it's in [3]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 of course; I didn't notice the last match.
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Adds an optional
*log.Entry
instance to thestellarCoreRunner
andCaptiveStellarCore
types so that we can process Captive Core's logs and redirect them to the Horizon log with the appropriate formatting / level / etc. The runner appends asubservice=captive-core
to the entries as outlined in #2669, making filtering easier.I've mapped
Error
andFatal
log levels toError
, while everything else maps to its equivalent. This is obviously trivial to change if we want to prioritize differently.Specifically, a log that used to look like this:
now looks like
Why
This closes #2669, which was re-opened to treat Core output like Horizon logs.
Known Limitations
Obviously, if Core's output format changes, the regex may need to be updated.